Skip to content

Modifies ROOT user related setup to Dockerfile.curobo#5376

Open
njawale42 wants to merge 10 commits intoisaac-sim:developfrom
njawale42:neel/dockerfile-curobo-remove-root-user
Open

Modifies ROOT user related setup to Dockerfile.curobo#5376
njawale42 wants to merge 10 commits intoisaac-sim:developfrom
njawale42:neel/dockerfile-curobo-remove-root-user

Conversation

@njawale42
Copy link
Copy Markdown
Contributor

@njawale42 njawale42 commented Apr 23, 2026

Description

Refactors docker/Dockerfile.curobo so the editable Isaac Lab installs and the final container runtime drop privileges to a non-root isaaclab user (uid/gid 1000) instead of staying as root the whole way. The cuRobo CI image (build-curobotest-curobo / test-skillgen) currently runs as root, with OMNI_KIT_ALLOW_ROOT=1 papering over the upstream Isaac Sim base image's intent of running non-root.

What changed

  1. Explicit USER root for the system-level setup block. apt, cuda-keyring repo, install_deps.py apt, ${DOCKER_USER_HOME} cache dirs, /bin/nvidia-* placeholders, the pre-bundled torch removal, and the get-pip.py bootstrap all require root and stay as root.

  2. New isaaclab user at uid/gid 1000. Created via explicit groupadd --non-unique --gid 1000 followed by useradd --non-unique --uid 1000 --gid 1000 isaaclab. The --non-unique flag on both is required because some base image revisions already provision a user/group at numeric ID 1000 (e.g. an ubuntu/kit-style account); the file system only cares about numeric IDs, so two names mapping to the same ID is fine. Pinning the GID explicitly via groupadd (instead of letting useradd auto-pick one when the default GID is taken) keeps the BuildKit pip cache mount's gid=1000 matched to the user's primary group. UID 1000 is chosen so that the runtime user matches the typical CI runner host user that owns the bind-mounted /workspace/isaaclab — required for the test runner's mkdir tests etc. to not fail with EACCES on the bind-mount overlay.

  3. chown + chmod + USER isaaclab before the pip installs.

    • The paths the new user needs to write to are chowned: ${ISAACLAB_PATH}, ${ISAACSIM_ROOT_PATH}/kit/python/.../site-packages, ${DOCKER_USER_HOME}, /home/isaaclab. /isaac-sim itself stays mode 755 and isaac-sim-owned by the base image — re-chowning it would force an OverlayFS copy-up of the multi-GB Isaac Sim install.
    • chmod 755 ${ISAACSIM_ROOT_PATH} opens up traversal of /isaac-sim for non-owner users. The base image creates /isaac-sim as isaac-sim's home directory; on recent Ubuntu (HOME_MODE 0700 in /etc/login.defs), that means mode 0700 isaac-sim:isaac-sim, which our isaaclab user cannot enter to find python.sh or the rest of the Kit runtime. Inner files retain their original perms (NVIDIA ships them world-readable per standard distribution conventions), so only the top-level directory's mode needs to change — single-inode metadata change, no recursive layer cost.
  4. BuildKit cache mount pinned to uid=1000,gid=1000 for ${DOCKER_USER_HOME}/.cache/pip, otherwise the default root-owned cache mount would be unwritable from isaaclab (BuildKit doesn't consult /etc/passwd at mount time, so the numeric form is required).

  5. ${HOME}/.bashrc used consistently in the alias RUN block. Two lines hardcoded /root/.bashrc while the surrounding eight used ${HOME}/.bashrc; after the user switch this inconsistency would silently land the trailing config in the wrong file.

  6. Final COPY annotated with --chown=isaaclab:isaaclab for ownership consistency.

CI build args (DOCKER_USER_HOME_ARG=/root in .github/actions/ecr-build-push-pull/action.yml, .github/actions/docker-build/action.yml, and .github/workflows/postmerge-ci.yml) are unchanged — /root is chowned to isaaclab inside the image, so they continue to work.

Type of change

  • CI/CD fix (non-breaking change which fixes an issue)

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

The NVIDIA Isaac Sim base image declares no USER directive and defaults
to root, so the explicit USER root in this Dockerfile is a no-op
declaration. Removing it aligns Dockerfile.curobo with the upstream
NVIDIA Isaac Sim Dockerfile and with the other Isaac Lab Dockerfiles
(Dockerfile.ros2, Dockerfile.installci) which omit the directive.

Build and runtime behavior are unchanged: the container still runs as
root via the base image default.
Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Isaac Lab Review Bot

Summary

This PR makes two minor cleanup changes to docker/Dockerfile.curobo: (1) removes a redundant USER root directive since the base image already defaults to root, and (2) changes two hardcoded /root/.bashrc references to use ${HOME}/.bashrc for consistency with the 8 preceding lines in the same RUN block. The changes are correct and improve maintainability.

Architecture Impact

Self-contained. This is a Docker build configuration change that affects only the cuRobo container image build process. No runtime code, no Isaac Lab framework code, and no other Dockerfiles are impacted. The container behavior remains identical since ${HOME} resolves to /root in the current configuration.

Implementation Verdict

Ship it

Test Coverage

No tests required. This is a Docker configuration cleanup with no behavioral change. The PR author's claim that "the container still builds and runs identically" is correct — both changes are purely cosmetic consistency improvements. Manual verification that the container builds successfully would be the appropriate validation, which is covered by standard CI Docker builds if available.

CI Status

No CI checks available yet. Once CI runs, the Docker build job (if present) would validate this change.

Findings

🔵 Improvement: docker/Dockerfile.curobo:34 — Removal of USER root is correct
The NVIDIA Isaac Sim base image does not set a non-root user, so this directive was indeed a no-op. Removing it aligns with the upstream Dockerfile style and reduces noise.

🔵 Improvement: docker/Dockerfile.curobo:170-171 — Consistent use of ${HOME} is correct
The change from /root/.bashrc to ${HOME}/.bashrc makes the entire RUN block internally consistent. While HOME=/root in the current build context, using the variable is more maintainable if the base image ever changes or if the Dockerfile is adapted for non-root users in the future.

Verification note: I confirmed that ${HOME} is the correct variable to use here by examining the full file context. The DOCKER_USER_HOME environment variable (line 33) is a different variable intended for bind-mount paths in Singularity (lines 116-123), while ${HOME} is the standard shell variable used throughout the alias block. The PR correctly uses ${HOME} to match the existing pattern.

No critical or warning-level issues identified. This is a clean, low-risk cleanup PR.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 23, 2026

Greptile Summary

This PR refactors docker/Dockerfile.curobo to create a non-root isaaclab user (uid/gid 1000) and drop privileges before the pip-based Isaac Lab installs, replacing the previous OMNI_KIT_ALLOW_ROOT=1 workaround. The approach correctly uses groupadd --non-unique + useradd --gid 1000 (avoiding the USERGROUPS_ENAB private-group trap), fixes the ${HOME} vs hardcoded /root/.bashrc inconsistency, and pins the BuildKit pip cache mount to uid=1000,gid=1000.

Confidence Score: 3/5

Merging is risky while two previously-flagged P1 issues — pip cache mount target pointing to /root/.cache/pip when the user's $HOME is /home/isaaclab, and the broader $HOME/$DOCKER_USER_HOME divergence — remain open and unaddressed in the current HEAD.

No new P0/P1 issues were found in this review pass; the single new finding is a P2 best-practice item (--no-log-init). However, two prior-thread P1 issues (pip cache mount no-op and HOME/DOCKER_USER_HOME mismatch) are still present in the code without resolution, keeping overall confidence below 4.

docker/Dockerfile.curobo — specifically the --mount=type=cache,target=${DOCKER_USER_HOME}/.cache/pip line (mount target vs. user $HOME) and the singularity directories created under ${DOCKER_USER_HOME} while the runtime user resolves ~ to /home/isaaclab.

Important Files Changed

Filename Overview
docker/Dockerfile.curobo Refactors curobo Dockerfile to drop privileges to non-root isaaclab (uid/gid 1000) before pip installs; open discussions from previous review around pip cache mount target mismatch and HOME/DOCKER_USER_HOME divergence remain unresolved.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["FROM base image\n(USER isaac-sim, uid 1234)"] --> B["USER root\n(system setup)"]
    B --> C["apt installs\nCUDA keyring\nnvidia-* placeholders\ntorch removal + pip reinstall"]
    C --> D["groupadd --non-unique --gid 1000 isaaclab\nuseradd --non-unique --uid 1000 --gid 1000 isaaclab"]
    D --> E["chown -R isaaclab:isaaclab\n  ISAACLAB_PATH\n  ISAACSIM_ROOT_PATH/kit/python\n  DOCKER_USER_HOME (=/root)\n  /home/isaaclab"]
    E --> F["chmod 755 ISAACSIM_ROOT_PATH\n(opens top-level traversal)"]
    F --> G["USER isaaclab\n(HOME=/home/isaaclab)"]
    G --> H["--mount=type=cache,\ntarget=DOCKER_USER_HOME/.cache/pip\n(=/root/.cache/pip) ⚠️\nuid=1000,gid=1000\n\nisaaclab.sh --install"]
    H --> I["pip uninstall quadprog\ncurobo install\nisaaclab_teleop install"]
    I --> J["Write aliases to HOME/.bashrc\n(=/home/isaaclab/.bashrc ✓)"]
    J --> K["COPY --chown=isaaclab:isaaclab\nall repo files into ISAACLAB_PATH"]
    K --> L["WORKDIR ISAACLAB_PATH"]

    style H fill:#ffe0cc,stroke:#d9534f
Loading

Reviews (8): Last reviewed commit: "Widens chown to /isaac-sim/kit/python so..." | Re-trigger Greptile

…ile.curobo

The base image (nvcr.io/nvidian/isaac-sim:latest-develop) ends with
USER isaac-sim (uid 1234). Previously the Dockerfile switched back to
root for the entire build and left runtime as root via
OMNI_KIT_ALLOW_ROOT. Removing the initial USER root broke CI because
apt needs root; simply restoring it keeps us as root the whole way
which is what we want to avoid.

Correct pattern applied here:

  * Explicit USER root for the system-level setup block (apt, cuda
    repo, /bin/nvidia-* placeholders, /root cache dirs, get-pip
    bootstrap, COPYed source files).
  * After the last root-required step, chown only the paths we are
    about to write to from the non-root user
    (${ISAACLAB_PATH}, site-packages, ${DOCKER_USER_HOME}) to
    isaac-sim:isaac-sim. /isaac-sim itself is already isaac-sim-owned
    from the base image, so re-chowning it would needlessly bloat
    the layer.
  * USER isaac-sim before the editable pip installs
    (isaaclab.sh --install, cuRobo, isaaclab_teleop) and the bashrc
    writes, so those paths end up owned by the runtime user.
  * BuildKit cache mount at ${DOCKER_USER_HOME}/.cache/pip pinned to
    uid=1234,gid=1234, otherwise it would default to root-owned and
    isaac-sim could not write into it.
  * Final COPY annotated with --chown=isaac-sim:isaac-sim for
    consistency.

This keeps CI build args (DOCKER_USER_HOME_ARG=/root) and
docker-compose paths unchanged, so docker-compose usage continues to
work with OMNI_KIT_ALLOW_ROOT. Dockerfile.base and Dockerfile.ros2
follow the same legacy pattern and are out of scope for this PR.
@njawale42
Copy link
Copy Markdown
Contributor Author

@greptileai

@njawale42 njawale42 changed the title Removes redundant USER root and inconsistent /root/.bashrc from Dockerfile.curobo ROOT user related modifications to Dockerfile.curobo Apr 23, 2026
… compat

The previous attempt switched to the base image's isaac-sim user (uid 1234),
which broke the test step in CI: the test runner bind-mounts the host
workspace (/opt/github-runner/_work/.../IsaacLab, owned by uid 1000) over
/workspace/isaaclab, overlaying our build-time chown. As uid 1234, the
container could not write to the bind mount and `mkdir tests` failed before
pytest even started.

Fix: create a fresh isaaclab user with uid 1000 to match the runner host
user, instead of remapping isaac-sim. Creating a new user (rather than
remapping isaac-sim's UID) avoids triggering an OverlayFS copy-up of the
multi-GB Isaac Sim install in /isaac-sim, which would otherwise become
orphan-owned. /isaac-sim stays mode 755 and isaac-sim-owned, so isaaclab
can still read and execute everything there. Only the paths the new user
needs to write to (${ISAACLAB_PATH}, kit site-packages, ${DOCKER_USER_HOME},
/home/isaaclab) are chowned.

Also bumps the BuildKit pip cache mount uid/gid from 1234 to 1000 (numeric
form is required because BuildKit does not consult /etc/passwd at mount
time) and switches the final COPY --chown to isaaclab.

Caveat: hardcodes uid 1000, matching the typical EC2 self-hosted runner
user. If a runner is reconfigured to a different uid this will break the
same way; in that case the right next step is to parameterize via build
arg, but until then 1000 is the simplest correct value.
Comment thread docker/Dockerfile.curobo Outdated

RUN chown -R isaaclab:isaaclab \
${ISAACLAB_PATH} \
${ISAACSIM_ROOT_PATH}/kit/python/lib/python3.12/site-packages \
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Hardcoded Python version will break on base-image upgrade

python3.12 is hard-coded in the chown path. If the upstream Isaac Sim base image ships Python 3.13 (or any other version), this path won't exist and the RUN chown -R layer will fail at build time with "No such file or directory," breaking the entire image build.

Use a glob or a shell expansion to resolve the versioned directory dynamically:

Suggested change
${ISAACSIM_ROOT_PATH}/kit/python/lib/python3.12/site-packages \
$(ls -d ${ISAACSIM_ROOT_PATH}/kit/python/lib/python3.*/site-packages 2>/dev/null | head -1) \

Comment thread docker/Dockerfile.curobo Outdated
@kellyguo11 kellyguo11 changed the title ROOT user related modifications to Dockerfile.curobo Modifies ROOT user related setup to Dockerfile.curobo Apr 26, 2026
kellyguo11 and others added 2 commits April 25, 2026 23:29
The pinned cuRobo CI base image (nvcr.io/nvidian/isaac-sim:latest-develop)
recently rolled forward to a digest that already provisions a user at
uid 1000, causing the previous `useradd ... --uid 1000 isaaclab` to fail
with `useradd: UID 1000 is not unique`.

Adds --non-unique (-o) so the new isaaclab user can share uid 1000 with
the existing one. File ownership only cares about the numeric uid, so
chown of the writable paths to isaaclab:isaaclab still produces uid 1000
files. Build-time ${HOME}/.bashrc writes still resolve to /home/isaaclab
because USER isaaclab triggers a getpwnam lookup by name, returning our
entry rather than the colliding one.
Comment thread docker/Dockerfile.curobo Outdated
`useradd --non-unique --uid 1000` only relaxes the UID uniqueness check;
it does not help with the implicit groupadd that useradd runs internally
to create the user's primary group. On base image revisions that already
carry a group at gid 1000 (likely whenever a uid-1000 user also exists),
that internal groupadd either fails outright or silently picks a different
gid like 1001, leaving the BuildKit pip cache mount's `gid=1000` mismatched
against the user's primary group.

Fix: explicitly groupadd the isaaclab group with --non-unique --gid 1000
before useradd, and pass --gid 1000 to useradd so it reuses the new group
as the user's primary group instead of auto-creating a per-user group.
This deterministically lands isaaclab at uid 1000 / gid 1000 regardless of
collisions in the base image.
The base image creates /isaac-sim as isaac-sim's home directory; on
recent Ubuntu (HOME_MODE 0700 in /etc/login.defs) that means mode 0700
isaac-sim:isaac-sim, which our new isaaclab user cannot enter. The
`[ -f $ISAACLAB_PATH/_isaac_sim/python.sh ]` test in isaaclab.sh then
fails, falls through to the system `python3` fallback, and the
isaaclab.sh --install step dies with `exec: python3: not found`.

Fix: chmod 755 /isaac-sim so non-owner users can traverse the directory
to reach python.sh and the rest of the Kit runtime. Files inside retain
their original permissions (NVIDIA ships them world-readable as part of
their standard distribution layout), so only the top-level directory's
mode needs to change. This is a single-inode metadata change; no
OverlayFS copy-up of the multi-GB tree.

If a future build surfaces a deeper permission issue inside /isaac-sim,
escalate to `chmod -R o+rX ${ISAACSIM_ROOT_PATH}` -- accepts the layer
cost in exchange for blanket access.
Comment thread docker/Dockerfile.curobo
Comment on lines +191 to 192
RUN --mount=type=cache,target=${DOCKER_USER_HOME}/.cache/pip,uid=1000,gid=1000 \
${ISAACLAB_PATH}/isaaclab.sh --install
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Pip cache mount target mismatches the isaaclab user's $HOME

After USER isaaclab, pip resolves its cache via ${HOME} — which points to /home/isaaclab (the user's home directory from /etc/passwd). The --mount target is ${DOCKER_USER_HOME}/.cache/pip which expands to /root/.cache/pip in CI. Pip will therefore write to /home/isaaclab/.cache/pip and never read from (or populate) the BuildKit cache mounted at /root/.cache/pip, making the cache layer a no-op.

The mount target should follow the user's actual $HOME:

RUN --mount=type=cache,target=/home/isaaclab/.cache/pip,uid=1000,gid=1000 \
    ${ISAACLAB_PATH}/isaaclab.sh --install

If keeping DOCKER_USER_HOME as the single source of truth is preferred, ENV HOME=${DOCKER_USER_HOME} (after updating DOCKER_USER_HOME_ARG to /home/isaaclab in CI) would reconcile both variables.

The previous chown only covered the python site-packages directory, but
pip also installs console_scripts and other binaries to the embedded
CPython's bin/ directory. The torch install pulls triton, which ships a
`proton` CLI binary that needs to land at
${ISAACSIM_ROOT_PATH}/kit/python/bin/proton -- a directory still owned
by isaac-sim (uid 1234) from the base image, so isaaclab cannot write
there and the editable install fails with EACCES.

Replaces the specific lib/python3.12/site-packages target with the
parent kit/python so chown covers bin/, lib/python3.X/{everything},
include/, share/, and any other subpath pip may write to as transitive
deps land. Drops the hardcoded python3.12 path from this chown step as
a side benefit; the rm-pip and get-pip lines elsewhere in this file
still hardcode 3.12 and are out of scope for this PR.

Layer cost: ~500 MB - 1 GB OverlayFS copy-up of the embedded CPython
tree. Accepted in exchange for not having to chase further pip-write
EACCES failures one transitive dep at a time.
Previously the user's home (/home/isaaclab from `useradd -m -d
/home/isaaclab`) diverged from ${DOCKER_USER_HOME} (=/root from CI build
arg). Consequence:

  * The BuildKit pip cache mount targets ${DOCKER_USER_HOME}/.cache/pip
    = /root/.cache/pip, but pip resolves the cache via $HOME/.cache/pip
    = /home/isaaclab/.cache/pip -- different paths, so the cache mount
    is a no-op and every CI build re-downloads everything.
  * The mkdir -p ${DOCKER_USER_HOME}/.{cache,nv,nvidia-omniverse,...}
    seeded the singularity-bind dirs at /root/* but Kit at runtime
    looks for them under $HOME = /home/isaaclab/*.

Fix: drop -m and pass -d ${DOCKER_USER_HOME} so isaaclab's home is
/root. Requires -M (no home creation) since /root already exists from
the base image; the chown step transfers ownership of it to isaaclab.
/home/isaaclab is no longer created and is removed from the chown
list. Now $HOME, the pip cache mount target, the singularity-bind
mkdirs, and the bashrc writes all resolve to the same /root path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants